fix(decisioning): project malformed-JSON 2xx responses to AdcpError#836
Merged
Conversation
When an upstream returns a 2xx status with a non-JSON body (e.g. a CDN or proxy returning an HTML error page), re-raise json.JSONDecodeError as AdcpError(SERVICE_UNAVAILABLE, recovery="transient") so adopters get a typed error consistent with the rest of the error-projection surface. Refs #453 https://claude.ai/code/session_018wq8sxhkG9c2WW4vgpYgS8
This was referenced May 23, 2026
![aao-ipr-bot[bot]](https://avatars.githubusercontent.com/u/134922?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. Right shape — mirrors the existing 5xx→SERVICE_UNAVAILABLE/transient projection at upstream.py:139-144 exactly, so the typed-error contract holds end-to-end. Adopters catching AdcpError no longer leak a ValueError subclass on the happy-path return.
Things I checked
AdcpError("SERVICE_UNAVAILABLE", message=..., recovery="transient")call shape matches the constructor attypes.py:109-128and the prior art atupstream.py:139-144.from excchains theJSONDecodeErrorinto__cause__— test asserts it attests/test_upstream_helpers.py:443.except ValueErroris the right catch:httpx.Response.json()documentsValueError;json.JSONDecodeErroris a subclass. Tighter than catchingException, looser than catching the subclass directly — correct trade-off.- Only one real
response.json()call site insrc/adcp/decisioning/(upstream.py:286). The match inproperty_list.py:57is docstring example code. No missed sites — no consistency leak. - 3.0 error-code enum has no
UPSTREAM_ERROR/INTERNAL_ERROR/GATEWAY_*member.SERVICE_UNAVAILABLEis the only spec-conformant projection for a 2xx-with-non-JSON-body in 3.0.INVALID_REQUESTwould falsely blame the buyer. - Message embeds
path,method, status, andstr(exc).JSONDecodeError.__str__reveals position/line only — no body bytes, no creds. Not a leak. - CI: 5 Python versions, Postgres conformance, four storyboard runners — all green.
Follow-ups (non-blocking — file as issues)
- 3.1
CONFIGURATION_ERROR/terminalprojection.schemas/cache/3.1.0-beta.3/enums/error-code.jsonaddsCONFIGURATION_ERROR(recovery:terminal) explicitly for "seller's deployment is misconfigured in a way that prevents handling the request — the buyer cannot fix it, retrying will not help." A CDN permanently serving HTML for a JSON endpoint is exactly that —transientwill have buyers retry-with-backoff into a brick wall. When the dispatch pins to 3.1+, branch this projection toCONFIGURATION_ERROR. Worth a TODO; not a blocker for 3.0. - Message hygiene.
_project_statusatupstream.py:112-114already has the right pattern:body_text[:200]snippet for the wire message, leaving parser internals out. The new branch instead concatenatesstr(exc)("Expecting value: line 1 column 1 (char 0)"). 3.1'sCONFIGURATION_ERRORdescription explicitly forbids stack traces / parser internals inerror.message. Switch to a boundedresponse.text[:200]snippet — more useful for adopters debugging the CDN page, and forward-compat with the 3.1 message-hygiene rule.__cause__keeps the decoder error for typed catches.
Minor nits (non-blocking)
- Test cleanup on assertion failure.
tests/test_upstream_helpers.py:434-443callsawait client.aclose()after the asserts, so a failing assert leaks the client. The other tests in the file use the same pattern, so this is internally consistent — flag it the next time the file gets a sweep, not now.
Approving on the strength of the exact-mirror of the existing 5xx projection plus a test that pins the __cause__ invariant. A 30-line fix to a one-line bug — the kind of PR that should sail through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #453
When an upstream returns a successful (2xx) status with a non-JSON body (e.g. a CDN or proxy returning an HTML error page),
UpstreamHttpClient._requestpreviously propagated a rawjson.JSONDecodeErrorfromresponse.json(). Adopters catchAdcpErrorthroughout; an unhandledValueErrorsubclass breaks that contract and is invisible in typed code. This PR wraps the call inexcept ValueErrorand re-raises asAdcpError(SERVICE_UNAVAILABLE, recovery="transient")— the same code used for 5xx responses, since an upstream returning HTML for a JSON endpoint is indistinguishable from a transient infrastructure failure.This is a re-cut of the original branch from #767 (
claude/issue-453-json-decode-error-handling), which was behind main and had maintainer edits disabled. The diff is identical to #767 but rebased cleanly on current main.What changed
src/adcp/decisioning/upstream.py: wrapresponse.json()intry/except ValueError; raiseAdcpError(SERVICE_UNAVAILABLE)withrecovery="transient"andfrom excchain.tests/test_upstream_helpers.py: addtest_200_with_malformed_json_raises_service_unavailable— mocks a 200 with an HTML body and asserts code, recovery, and__cause__.What tested
pytest tests/test_upstream_helpers.py -q— 35 passed, 0 failedmypy src/adcp/decisioning/upstream.py— cleanruff check src/adcp/decisioning/upstream.py tests/test_upstream_helpers.py— cleanPre-PR review
@pytest.mark.asyncio(false positive:asyncio_mode = "auto"in pyproject.toml); one nit on truncatingexcin message (low severity,AdcpErroris internal)SERVICE_UNAVAILABLE / transientis correct for CDN/proxy interception; nit on adding boundedresponse.text[:200]snippet to message; no blockersNits (not fixed):
exctext rather than a body-length hint; useful for debugging, acceptable for an internal error typerecovery="transient"is correct per the spec but can't distinguish "CDN blip" from "permanently misconfigured upstream" — no better enum value existsSession: https://claude.ai/code/session_018wq8sxhkG9c2WW4vgpYgS8
Generated by Claude Code